-
Notifications
You must be signed in to change notification settings - Fork 519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for qcow2 as an image format. #1425
Conversation
tools/rpm2img
Outdated
chown 1000:1000 "${OUTPUT_DIR}/${DISK_IMAGE_BASENAME}.${IMAGE_EXTENSION}" \ | ||
"${OUTPUT_DIR}/${DATA_IMAGE_BASENAME}.${IMAGE_EXTENSION}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just chown
everything under ${OUTPUT_DIR}
- then we don't need to keep track of IMAGE_EXTENSION
, and we only need to run it once at the very end.
chown 1000:1000 "${OUTPUT_DIR}/${DISK_IMAGE_BASENAME}.${IMAGE_EXTENSION}" \ | |
"${OUTPUT_DIR}/${DATA_IMAGE_BASENAME}.${IMAGE_EXTENSION}" | |
find "${OUTPUT_DIR}" -type f -print -exec chown 1000:1000 {} \; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thanks for the comments. I was going for the minimum change, I'll take a look at the proposed comments tomorrow and incorporate them. |
fi | ||
|
||
find "${OUTPUT_DIR}" -type f -print -exec chown 1000:1000 {} \; | ||
|
||
lz4 -9vc "${BOOT_IMAGE}" >"${OUTPUT_DIR}/${BOOT_IMAGE_NAME}" | ||
lz4 -9vc "${VERITY_IMAGE}" >"${OUTPUT_DIR}/${VERITY_IMAGE_NAME}" | ||
lz4 -9vc "${ROOT_IMAGE}" >"${OUTPUT_DIR}/${ROOT_IMAGE_NAME}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can move the find
command below these lz4 steps, and delete the other chown
command since it's redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
LGTM apart from the nitpick. Can you squash your two commits together before we merge? |
qcow2 is pretty common in terms of image formats. Its what OpenStack, KVM, and so on use as their VM image format. This patch adds support for creating images in that format. I have verified the output images by booting them on a KVM system.
377f9d4
to
652228e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍰
Thanks again for the contribution! Nice work. Just for future reference and since you had a question about it:
|
Issue number:
None, should every PR have an associated issue? CONTRIBUTING.md appears ambigious on this point.
Description of changes:
Add support for qcow2 as an output format for iamges. qcow2 is pretty common in terms of image formats. Its what OpenStack, KVM, and so on use as their VM image format. This patch adds support
for creating images in that format.
Testing done:
I have verified the output images by booting them on a KVM system.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.